Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor experiment.set_counter() and some minor bug fixes #366

Merged
merged 3 commits into from
Oct 4, 2023

Conversation

jo-basevi
Copy link
Collaborator

  • Fix to payu crashing with empty restart files/dirs
  • Add default values of LD_LIBRARY_PATHS
  • Raise exceptions for missing model executables

Closes #285, closes #307, closes #306

@pep8speaks
Copy link

pep8speaks commented Sep 27, 2023

Hello @jo-basevi! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-10-03 23:26:51 UTC

@coveralls
Copy link

coveralls commented Sep 27, 2023

Coverage Status

coverage: 42.053% (+0.04%) from 42.016% when pulling b5c0c30 on jo-basevi:285-fix-crash-with-restart into edf8d8c on payu-org:master.

Copy link
Collaborator

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for cleaning that up and closing a bunch of issues.

Just a couple of tweaks and it should be good to go.

payu/experiment.py Show resolved Hide resolved
payu/models/model.py Outdated Show resolved Hide resolved
payu/models/model.py Outdated Show resolved Hide resolved
@jo-basevi
Copy link
Collaborator Author

A previous CI action failed at the lint stage because of payu/experiment.py:564:20: E1111: Assigning result of a function call, where the function has no return (assignment-from-no-return) where the code was in the payu run command:

for prof in self.profilers:
      if prof.runscript:
          model_prog = model_prog.append(prof.runscript)

I think it found a legitimate error so I fixed it in the previous commit (though could be wrong). But I am bit confused as to how that error isn't picked up when running pylint --extension-pkg-whitelist=netCDF4 --ignored-modules=backports -E payu locally or in other CI builds in other PRs..

@aidanheerdegen
Copy link
Collaborator

I think it found a legitimate error so I fixed it in the previous commit (though could be wrong). But I am bit confused as to how that error isn't picked up when running pylint --extension-pkg-whitelist=netCDF4 --ignored-modules=backports -E payu locally or in other CI builds in other PRs..

Yeah that is weird. It is a not often used part of the code, but you're right that a limiter should have picked that up before. Is computing supposed to be deterministic?

@jo-basevi
Copy link
Collaborator Author

Yeah that is weird. It is a not often used part of the code, but you're right that a limiter should have picked that up before. Is computing supposed to be deterministic?

Well it is now failing in another PR, so it'll be good to fix here..

Jo Basevi added 3 commits October 4, 2023 10:26
- Fix to payu crashing with empty restart files/dirs
- Add default values of LD_LIBRARY_PATHS
- Raise exceptions for missing model executables
@jo-basevi jo-basevi marked this pull request as ready for review October 3, 2023 23:26
@aidanheerdegen aidanheerdegen self-requested a review October 4, 2023 01:00
Copy link
Collaborator

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to merge I think. Thanks.

@jo-basevi jo-basevi merged commit fc83ec9 into payu-org:master Oct 4, 2023
8 checks passed
@jo-basevi jo-basevi deleted the 285-fix-crash-with-restart branch October 4, 2023 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants